Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e74fe4221
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73dd5638db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| #[cfg(not(target_os = "windows"))] | ||
| pub fn sandbox_setup_is_complete(_codex_home: &Path) -> bool { | ||
| false |
There was a problem hiding this comment.
Should this return true? There is no need to do windows sandbox setup on other platforms, so it is compete in a technical/literal sense. Less technically speaking, if a check on this condition leaks into linux/macos code, we probably don't want to follow the path more likely to to invoke run_elevated_setup
There was a problem hiding this comment.
yeah I'll update this in a future PR. For now there is no leakage
| cfg!(target_os = "windows") | ||
| && codex_core::windows_sandbox::ELEVATED_SANDBOX_NUX_ENABLED | ||
| && codex_core::get_platform_sandbox().is_some() | ||
| && !codex_core::is_windows_elevated_sandbox_enabled() |
There was a problem hiding this comment.
This feels like we're piling tech debt on top of the "2 booleans" problem
There was a problem hiding this comment.
agreed, will address in a future PR
| use std::time::Duration; | ||
| use std::time::Instant; | ||
|
|
||
| fn windows_degraded_sandbox_active() -> bool { |
There was a problem hiding this comment.
Can we s/degraded/partial/g ?
| #[cfg(not(target_os = "windows"))] | ||
| { | ||
| Self::approval_preset_actions(preset.approval, preset.sandbox.clone()) | ||
| } |
There was a problem hiding this comment.
nit: can we bump the non-windows variant to be first here?
| preset: preset_clone.clone(), | ||
| }); | ||
| })] | ||
| } |
There was a problem hiding this comment.
[non-blocking] I know this PR is not the source of the problem, but I feel like we're reaching a tipping point with the complexity in this function - we're iterating over the presets as if we're agnostic, but almost all of the logic in the function is targeted at specific variants. Seems like we'd be better off with a more declarative pattern.
| @@ -3108,36 +3183,106 @@ | |||
| pub(crate) fn open_windows_sandbox_enable_prompt(&mut self, preset: ApprovalPreset) { | |||
There was a problem hiding this comment.
could we move this function out of chatwidget into its own module?
There was a problem hiding this comment.
will do in a future PR
| pub(crate) fn open_windows_sandbox_enable_prompt(&mut self, _preset: ApprovalPreset) {} | ||
|
|
||
| #[cfg(target_os = "windows")] | ||
| pub(crate) fn open_windows_sandbox_fallback_prompt( |
There was a problem hiding this comment.
ditto above, separate module for this code would be really helpful
There was a problem hiding this comment.
will do in a future PR
| self.chat_widget | ||
| .open_windows_sandbox_fallback_prompt(preset, reason); | ||
| } | ||
| AppEvent::BeginWindowsSandboxElevatedSetup { preset } => { |
There was a problem hiding this comment.
can we wrap this handler logic into a function similar to open_windows_sandbox_fallback_prompt?
There was a problem hiding this comment.
will do in a future PR
73dd563 to
98aa544
Compare
c8e5101 to
4111eae
Compare
|
@codex review again please! |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Elevated Sandbox NUX: